config/v1/types_infrastructure.go: add service endpoints to aws platform status #599

abhinavdahiya · 2020-03-13T16:32:49Z

The change is based on the aws-custom-region-custom-endpoints enhancement. This change only adds the status update because the spec update requires the work of
addtional operator that will be added later on, and therefore that change will be added when the work for operator is in flight and this change allows operators to have a head start for that time.

abhinavdahiya · 2020-03-13T16:33:29Z

/assign @soltysh

deads2k · 2020-03-17T20:48:21Z

Before this merges, I think the related enhancement needs to reach closure. Creating an api that requires stitching in at least four different operators seems like a bad pattern to create without buy in from those components. In addition, the user input here would result in either an improper cloud.conf being provided to those components or information duplication.

If you provided an operator/cluster-config-operator control loop to perform the stitching it would make this much more palatable.

/hold

abhinavdahiya · 2020-03-17T21:04:33Z

Before this merges, I think the related enhancement needs to reach closure. Creating an api that requires stitching in at least four different operators seems like a bad pattern to create without buy in from those components. In addition, the user input here would result in either an improper cloud.conf being provided to those components or information duplication.

If you provided an operator/cluster-config-operator control loop to perform the stitching it would make this much more palatable.

/hold

from the PR description..

This change only adds the status update because the spec update requires the work of
addtional operator that will be added later on, and therefore that change will be added when the work for operator is in flight and this change allows operators to have a head start for that time.

sdodson · 2020-03-31T14:43:21Z

The enhancement was approved, do we need to update anything here to sync with changes or is this ready to go? @abhinavdahiya @deads2k

deads2k · 2020-03-31T19:15:48Z

config/v1/types_infrastructure.go

+	// platformSpec holds desired information specific to the underlying
+	// infrastructure provider.
+	// +optional
+	PlatformSpec *PlatformSpec `json:"platformSpec,omitempty"`


I think an empty type implies missing. We try to avoid pointers.

since go marshal/unmarshal doesn't omit empty structs, I think pointer is a better choice here.

Also this is important for migrations as detailed in https://github.com/openshift/api/pull/599/files#r401189373

since go marshal/unmarshal doesn't omit empty structs, I think pointer is a better choice here.

We are ok with including empty values in serialization. In some ways it's even good since it outlines what is available and what is not. We've been avoiding pointer in our config structs to good effect. If migration cannot be handled another way we would do it, but I don't think that's the case today.

By allowing empty string in Type, you can distinguish intent.

deads2k · 2020-03-31T19:16:36Z

config/v1/types_infrastructure.go

+	// type is the underlying infrastructure provider for the cluster. This
+	// value controls whether infrastructure automation such as service load
+	// balancers, dynamic volume provisioning, machine creation and deletion, and
+	// other integrations are enabled. If None, no infrastructure automation is


how will you migrate existing clusters on update?

@deads2k
personally, the platformSpec should be empty nil <=> no configuration for infra like we support today.
and when users want to modify existing clusters to add infra specific desired state, the validations should force them to set the type and corresponding platformSpec.

But if you think emptying out the spec to just platform type and empty platform spec is more suitable, which i don't think we should be doing, we (same team that is doing the platformstatus migration) can write the migration to set it.

secondary, there is planned change to validations (in openshift kube-apiserver) to make sure the type doesn't change once set. i.e when the platformSpec is once set, it can't change the type.

deads2k · 2020-03-31T19:17:19Z

config/v1/types_infrastructure.go

+	// This must be provided and cannot be empty.
+	Name string `json:"name"`
+
+	// URL is hostname only or fully qualified URI that overrides the default generated


do we allow http or only https? I strongly prefer the latter.

personally that's the restriction from AWS SDK, and i'm tracking that here. it doesn't say http or https only...

personally that's the restriction from AWS SDK, and i'm tracking that here. it doesn't say http or https only...

We get to be opinionated in our API. If calls should only be https, then we should restrict them. The doc doesn't match the field name. why hostname only in a url field?

deads2k · 2020-03-31T19:18:18Z

config/v1/types_infrastructure.go

+// AWSServiceEndpoint store the configuration of a custom url to
+// override existing defaults of AWS Services.
+type AWSServiceEndpoint struct {
+	// Name is the name of the AWS service.


we need to restrict these names. enumerated keys would be best. Name restrictions would be second best.

The names cannot be restricted. The number of AWS services are unbounded, and we can't play catchup with AWS SDK here. The requirement is users should be able to set endpoints for any AWS service.

We need to at least document a naming convention for AWS services. We can't have customers define S3 as the service name, and have things like the image registry not pick up the endpoint because its operator is expecting s3 as the service name...

We need to at least document a naming convention for AWS services. We can't have customers define S3 as the service name, and have things like the image registry not pick up the endpoint because its operator is expecting s3 as the service name...

that's what i'm saying, AWS controls the names of the services and not us... so personally i don't really know how we can unless we wanna play catch up all the time..

?? open to suggestions that don't end up hard coding the list.

Here's an idea - for a given AWS service:

Drop any "AWS" or "Amazon" prefix.

Drop any "Service" suffix.

If the service has a well-established acronym in AWS documentation, use that in all capitals. Example: EC2, S3, ELB, VPC, RDS.

Otherwise, use the service name in CamelCase. Ex: Route53, ElastiCache. Names should have a leading capital letter.

@adambkaplan please no, AWS services already have names in their SDK:

https://github.com/aws/aws-sdk-go/blob/fa81b5cb4456a1e77f5ccd74d729d1761821aea7/models/endpoints/endpoints.json#L68

+1 @dmage - seems like this is the canonical list of endpoints. Make sense for us to out-link to their SDK source?

It makes sense to reference. A quick scan seems to reveal that they don't support emoji characters. Anything other than a-z,-,0-9? We can start restrictive, but catching obvious typos like that seems like a good idea.

config/v1/types_infrastructure.go

deads2k · 2020-03-31T19:24:04Z

config/v1/types_infrastructure.go

+// PlatformSpec holds the desired state specific to the underlying infrastructure provider
+// of the current cluster. Since these are used at spec-level for the underlying cluster, it
+// is supposed that only one of the spec structs is set.
+type PlatformSpec struct {


indicate how these are vetted, validated, and promoted.

these

wdym by these? individual platform specs or something else.

abhinavdahiya · 2020-04-03T18:24:29Z

ping @deads2k @sttts for review

sttts · 2020-04-06T15:18:26Z

/assign @deads2k

bparees · 2020-04-13T13:48:30Z

@abhinavdahiya @deads2k any idea when this will land? Downstream operators are expected to consume this in 4.5 to fulfill https://issues.redhat.com/browse/CORS-1271 right?

deads2k · 2020-04-13T16:19:11Z

config/v1/types_infrastructure.go

+// AWSServiceEndpoint store the configuration of a custom url to
+// override existing defaults of AWS Services.
+type AWSServiceEndpoint struct {
+	// Name is the name of the AWS service.


deads2k · 2020-04-13T16:21:34Z

config/v1/types_infrastructure.go

+	// This must be provided and cannot be empty.
+	Name string `json:"name"`
+
+	// URL is hostname only or fully qualified URI that overrides the default generated


doc matches tag: url

deads2k · 2020-04-13T16:23:13Z

config/v1/types_infrastructure.go

+	// URL is hostname only or fully qualified URI that overrides the default generated
+	// endpoint for a client.
+	// This must be provided and cannot be empty.
+	URL string `json:"url"`


@damemi do we default to required or optional? Is there any downside to simply specifying everything?

config/v1/types_infrastructure.go

deads2k · 2020-04-13T18:36:27Z

config/v1/types_infrastructure.go

 	Name string `json:"name"`

-	// URL is hostname only or fully qualified URI that overrides the default generated
+	// url is hostname only or fully qualified URI with schemes https or http, that overrides the default generated


What requires HTTP? Do we really want to allow that. If we don't know of anything, let's require secure to match the posture of the rest of the product.

What requires HTTP? Do we really want to allow that. If we don't know of anything, let's require secure to match the posture of the rest of the product.

AWS SDK supports it, and that's way I have it here, keeping the parity with AWS SDK.

dropped it in 1bd3edd

deads2k · 2020-04-13T19:45:47Z

/lgtm
/hold

squash please, then retag at will.

…orm spec and status The change is based on the aws-custom-region-custom-endpoints enhancement [1]. This change only addresses the service endpoints list in the infrastructure object. The service endpoints URL currently only support `https` schemes to follow openshift security stance. [1]: https://github.com/openshift/enhancements/pull/163/files#diff-b6a7e34af40d81a977a31247b107348eR191-R203

deads2k · 2020-04-13T20:02:22Z

/lgtm
/hold cancel

openshift-ci-robot · 2020-04-13T20:02:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, deads2k

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ud-config ConfigMap cloudConfig is now generated by kube_cloud_config controller for all supported platforms. Controller generates kube-cloud-config ConfigMap in openshift-config-managed namespace where cloud.conf key is stored. Links: - https://github.com/openshift/enhancements/blob/master/enhancements/installer/aws-custom-region-and-endpoints.md - openshift/api#599 - openshift/api#621

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 13, 2020

openshift-ci-robot requested review from adambkaplan and deads2k March 13, 2020 16:33

openshift-ci-robot assigned soltysh Mar 13, 2020

abhinavdahiya force-pushed the aws_service_endpoints branch from 1881232 to 2e7af5c Compare March 13, 2020 16:50

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 13, 2020

abhinavdahiya mentioned this pull request Mar 17, 2020

AWS: support custom regions and custom endpoints. openshift/installer#3277

Merged

10 tasks

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 17, 2020

abhinavdahiya force-pushed the aws_service_endpoints branch 2 times, most recently from 529b4b8 to 332b556 Compare March 31, 2020 02:01

deads2k reviewed Mar 31, 2020

View reviewed changes

config/v1/types_infrastructure.go Show resolved Hide resolved

deads2k reviewed Mar 31, 2020

View reviewed changes

abhinavdahiya mentioned this pull request Mar 31, 2020

aws_platform_service_location: add a controller that sync user specified service endpoints to status openshift/cluster-config-operator#119

Merged

openshift-ci-robot assigned deads2k Apr 6, 2020

abhinavdahiya mentioned this pull request Apr 6, 2020

config/v1/types_infrastructure.go: add kube cloud config to status #621

Merged

deads2k reviewed Apr 13, 2020

View reviewed changes

abhinavdahiya force-pushed the aws_service_endpoints branch from 332b556 to f3bb448 Compare April 13, 2020 17:46

deads2k reviewed Apr 13, 2020

View reviewed changes

config/v1/types_infrastructure.go Show resolved Hide resolved

deads2k reviewed Apr 13, 2020

View reviewed changes

abhinavdahiya force-pushed the aws_service_endpoints branch from 1bd3edd to 3428c53 Compare April 13, 2020 18:57

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 13, 2020

abhinavdahiya force-pushed the aws_service_endpoints branch from 5dbb30e to 6c25f0f Compare April 13, 2020 19:59

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Apr 13, 2020

openshift-merge-robot merged commit c6e8c9b into openshift:master Apr 13, 2020

sinnykumari mentioned this pull request Apr 16, 2020

Bug 1825948: cloudConfig: read cloud Config from openshift-config-managed/kube-cloud-config ConfigMap openshift/machine-config-operator#1658

Merged

config/v1/types_infrastructure.go: add service endpoints to aws platform status #599

config/v1/types_infrastructure.go: add service endpoints to aws platform status #599

Uh oh!

Conversation

abhinavdahiya commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhinavdahiya commented Mar 13, 2020

Uh oh!

deads2k commented Mar 17, 2020

Uh oh!

abhinavdahiya commented Mar 17, 2020

Uh oh!

sdodson commented Mar 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adambkaplan Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Apr 3, 2020

Uh oh!

sttts commented Apr 6, 2020

Uh oh!

bparees commented Apr 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Apr 13, 2020

Uh oh!

deads2k commented Apr 13, 2020

Uh oh!

openshift-ci-robot commented Apr 13, 2020

Uh oh!

Reviewers

abhinavdahiya commented Mar 13, 2020 •

edited

Loading

adambkaplan Apr 6, 2020 •

edited

Loading